Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dont show expansion for empty actions steps #29977

Merged
merged 10 commits into from Mar 24, 2024
Merged

Dont show expansion for empty actions steps #29977

merged 10 commits into from Mar 24, 2024

Conversation

silverwind
Copy link
Member

This hides the chevron icon and makes the step header unclickable for skipped steps because there is no content to expand on those.

Before:

Screenshot 2024-03-21 at 20 06 47

After:
Screenshot 2024-03-21 at 20 03 07

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 21, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 21, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 21, 2024
@silverwind
Copy link
Member Author

silverwind commented Mar 21, 2024 •
edited

There was a bug because tw-w-4 resolved to 14px instead of the 16px that I expected because it's using rem units but out root element has 14px, so pixels sizes in tailwind docs currently do not apply to us.

Screenshot 2024-03-21 at 20 16 10

Edit: found a better solution.

@silverwind
Copy link
Member Author

silverwind commented Mar 21, 2024 •
edited

There are likely more statuses that should not be expandable, but I need some help to know which ones out of these have no output and should therefor not be expandable:

unknown <- no output?
waiting <- no output?
running
success
failure
cancelled
skipped <- done
blocked <- no output?

Maybe @wolfogre would know.

@silverwind silverwind changed the title Handle skipped action step status in UI Prevent expansion of skipped action steps Mar 21, 2024
@silverwind
Copy link
Member Author

silverwind commented Mar 21, 2024 •
edited

I made it now like below, I think it's likely correct, but confirmation would still be great. I'm especially uncertain about blocked and unknown.

running - expandable
success - expandable
failure - expandable
cancelled - expandable
skipped - not expandable
blocked - not expandable
unknown - not expandable
waiting - not expandable

@silverwind silverwind changed the title Prevent expansion of skipped action steps Dont show expansion for empty actions steps Mar 22, 2024
@wolfogre
Copy link
Member

wolfogre commented Mar 22, 2024 •
edited

I made it now like below, I think it's likely correct, but confirmation would still be great. I'm especially uncertain about blocked and unknown.

running - expandable
success - expandable
failure - expandable
cancelled - expandable
skipped - not expandable
blocked - not expandable
unknown - not expandable
waiting - not expandable

I believe it's entirely correct. There absolutely cannot be any logs for statuses skipped, blocked and waiting. unknown shouldn't be passed to users, if it happens, it's a bug, so it's OK to either make it expandable or not.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 24, 2024
@denyskon denyskon added this to the 1.22.0 milestone Mar 24, 2024
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 24, 2024
@silverwind silverwind enabled auto-merge (squash) March 24, 2024 13:33
@silverwind silverwind merged commit 5bd0773 into go-gitea:main Mar 24, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 24, 2024
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 24, 2024
@silverwind
Merge remote-tracking branch 'origin/main' into twgap
72da884 
* origin/main:
  Dont show expansion for empty actions steps (go-gitea#29977)
  Remove fomantic header module (go-gitea#30033)
silverwind added a commit to silverwind/gitea that referenced this pull request Mar 24, 2024
@silverwind
gerge remote-tracking branch 'origin/main' into rmcontainer
2d9952c 
* origin/main:
  Dont show expansion for empty actions steps (go-gitea#29977)
  Remove fomantic header module (go-gitea#30033)
@silverwind silverwind deleted the skipped branch March 27, 2024 01:56
@silverwind silverwind mentioned this pull request Mar 27, 2024
Merged
lunny pushed a commit that referenced this pull request Mar 27, 2024
@silverwind
Fix click handler in job-step-summary (#30122)
57539bc 
Fix mistake from #29977 where the
click handler wasn't updated for the change with the `isExpandable`
function.
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Mar 30, 2024
@silverwind @earl-warren
Fix click handler in job-step-summary (#30122)
43db1e8 
Fix mistake from go-gitea/gitea#29977 where the
click handler wasn't updated for the change with the `isExpandable`
function.

(cherry picked from commit 57539bcdc024110c890320e3e785bf3d6ad6df55)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denyskon denyskon denyskon approved these changes

@delvh delvh delvh approved these changes

@wolfogre wolfogre Awaiting requested review from wolfogre

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/js size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@silverwind @wolfogre @denyskon @delvh @yardenshoham @GiteaBot